Load predefined users from a JSON file through command line. #9229#9652
Merged
khushboovashi merged 8 commits intopgadmin-org:masterfrom Feb 24, 2026
Merged
Load predefined users from a JSON file through command line. #9229#9652khushboovashi merged 8 commits intopgadmin-org:masterfrom
khushboovashi merged 8 commits intopgadmin-org:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a JSON-driven bulk user import CLI command ( Changes
Sequence DiagramsequenceDiagram
actor CLI as User/CLI
participant File as JSON File
participant Parser as JSON Parser
participant Importer as ManageUsers.load_users
participant Roles as ManageRoles
participant DB as Database
CLI->>File: open input_file
File-->>Parser: read & parse JSON -> users[]
Parser-->>Importer: provide users list
loop for each user
Importer->>Importer: validate fields & password rules
Importer->>DB: lookup by username/email
DB-->>Importer: exists? (yes/no)
alt not exists
Importer->>Roles: resolve role names -> IDs
Roles-->>Importer: role IDs
Importer->>DB: create user record
DB-->>Importer: created
else exists
Importer-->>Importer: mark skipped
end
end
Importer-->>CLI: report counts (created, skipped, errors)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
web/setup.py (3)
246-257: No validation ofauth_sourcevalues from user-supplied JSON.The
auth_sourcefield is taken verbatim from the JSON (line 247). If a user provides an unrecognized value (e.g.,"Internal"capitalized, or a typo like"interanl"), it silently passes through andcreate_userwill attempt to create the user with an invalid auth source. Consider validating against the known constants (INTERNAL,LDAP,OAUTH2,KERBEROS,WEBSERVER) and reporting an error for unrecognized values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/setup.py` around lines 246 - 257, The code assigns auth_source directly from user_entry into user_data without validating it; update the block that reads auth_source (the user_entry.get('auth_source', INTERNAL) and the user_data dict) to validate the value against the allowed constants (INTERNAL, LDAP, OAUTH2, KERBEROS, WEBSERVER) and normalize casing if desired, and if the provided value is not one of these, raise or return a clear error before calling create_user so invalid auth_source values (e.g., "Internal" or typos) are rejected and only recognized constants are used.
209-211:open()without explicit encoding.
open(file_path)uses the platform default encoding, which may not be UTF-8 on all systems (notably Windows). Since JSON is typically UTF-8:- with open(file_path) as f: + with open(file_path, encoding='utf-8') as f:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/setup.py` around lines 209 - 211, The file opens JSON files using open(file_path) without an explicit encoding; update the code that reads into jsonlib.load to open the file with an explicit UTF-8 encoding (i.e., pass encoding="utf-8" to open) so reads are deterministic across platforms—locate the try block that uses file_path and jsonlib.load and change the open call accordingly.
289-295: Useconfig.PASSWORD_LENGTH_MINinstead of hardcoded6for consistency with the rest of the codebase.Password validation elsewhere in the codebase (e.g.,
web/pgadmin/setup/user_info.py,web/pgadmin/browser/__init__.py) consistently usesconfig.PASSWORD_LENGTH_MIN. Hardcoding6here means this validation will drift if the configured minimum changes.Since
configis already imported, update the check and error message:♻️ Proposed fix
if auth_source == INTERNAL: - if len(user_data['newPassword']) < 6: + if len(user_data['newPassword']) < \ + config.PASSWORD_LENGTH_MIN: print(f"Skipping user '{user_data['username']}': " - f"password must be at least 6 characters") + f"password must be at least " + f"{config.PASSWORD_LENGTH_MIN} characters")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/setup.py` around lines 289 - 295, Replace the hardcoded minimum password length 6 with the configured constant by using config.PASSWORD_LENGTH_MIN in the validation and the error message: in the block that checks auth_source == INTERNAL and inspects user_data['newPassword'] (and increments error_count/continues), change the numeric literal to config.PASSWORD_LENGTH_MIN and update the printed string to interpolate that constant so the message reflects the configured minimum.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/setup.py`:
- Around line 166-170: The load_users command currently accepts a json parameter
but never uses it; update the load_users function to honor the json flag (or
remove the parameter) — specifically, in the load_users function implement
conditional output: after loading users, if json is True serialize the resulting
users list to JSON (matching the format used by the get-users command) and print
to stdout (or write to the same destination get-users uses for JSON), otherwise
keep the existing human-readable output; keep the `@update_sqlite_path` decorator
as-is and use the existing user-loading logic in load_users to produce the
object you will JSON-serialize.
- Around line 202-219: Remove the redundant print of the exception in the
unquote error path: when unquote(input_file) raises, stop calling print(str(e))
and instead directly return _handle_error(str(e), True) (same behavior as the
JSON error branches). Update the try/except around unquote to catch Exception as
e, assign file_path = unquote(input_file) in the try, and in the except simply
return _handle_error(str(e), True) (references: unquote, input_file, file_path,
_handle_error).
- Around line 240-244: The print statement in the user import block (where
user_entry is validated) uses an f-string with no placeholders and doesn't
identify which entry failed; update the message to include context (e.g., the
user_entry contents or its index) so failures are debuggable and to satisfy Ruff
F541. Locate the validation around the user_entry variable in web/setup.py (the
block that checks 'username' and 'email') and change the print to include either
the current index from an enumerate or the user_entry dict (or both) in the
output, and keep incrementing error_count and continue as before.
- Around line 270-285: Replace calls to ManageUsers.get_user and
ManageRoles.get_role inside load_users with direct ORM queries against the User
and Role models using the existing app context (avoid calling
create_app()/test_request_context inside ManageUsers/ManageRoles to eliminate
per-user app creation); also change the f-string "f\"Skipping user:
missing...\"" to a plain string, replace the hardcoded password min length 6
with config.PASSWORD_LENGTH_MIN, open files with explicit encoding (e.g.
open(file_path, encoding="utf-8")), remove the redundant explicit print of
errors so only _handle_error prints/logs them, and remove the unused json
parameter from the function signature and any callers. Ensure you reference
ManageUsers.get_user and ManageRoles.get_role to locate the affected calls and
update load_users and the function definition that has the json parameter.
---
Nitpick comments:
In `@web/setup.py`:
- Around line 246-257: The code assigns auth_source directly from user_entry
into user_data without validating it; update the block that reads auth_source
(the user_entry.get('auth_source', INTERNAL) and the user_data dict) to validate
the value against the allowed constants (INTERNAL, LDAP, OAUTH2, KERBEROS,
WEBSERVER) and normalize casing if desired, and if the provided value is not one
of these, raise or return a clear error before calling create_user so invalid
auth_source values (e.g., "Internal" or typos) are rejected and only recognized
constants are used.
- Around line 209-211: The file opens JSON files using open(file_path) without
an explicit encoding; update the code that reads into jsonlib.load to open the
file with an explicit UTF-8 encoding (i.e., pass encoding="utf-8" to open) so
reads are deterministic across platforms—locate the try block that uses
file_path and jsonlib.load and change the open call accordingly.
- Around line 289-295: Replace the hardcoded minimum password length 6 with the
configured constant by using config.PASSWORD_LENGTH_MIN in the validation and
the error message: in the block that checks auth_source == INTERNAL and inspects
user_data['newPassword'] (and increments error_count/continues), change the
numeric literal to config.PASSWORD_LENGTH_MIN and update the printed string to
interpolate that constant so the message reflects the configured minimum.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/pgadmin/utils/session.py (1)
105-105:⚠️ Potential issue | 🟡 MinorPipeline E302: add a second blank line before
class CachingSessionManager.pycodestyle requires two blank lines between top-level definitions; only one is present here.
🐛 Proposed fix
def put(self, session): 'Store a managed session' raise NotImplementedError + class CachingSessionManager(SessionManager):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/utils/session.py` at line 105, Add one more blank line immediately before the declaration of class CachingSessionManager (the class that subclasses SessionManager) so there are two blank lines separating this top-level class definition from the previous top-level code; this will satisfy pycodestyle E302 by ensuring two blank lines before the CachingSessionManager class declaration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/pgadmin/utils/session.py`:
- Line 161: The conditional "if self.is_session_ready(session) and
session.hmac_digest != digest:" in session.py exceeds the 79-char limit; split
it across multiple lines by adding parentheses or temporary booleans to wrap the
two checks (e.g., assign self.is_session_ready(session) to a variable like ready
or place the two expressions on separate lines within parentheses) so the
condition reads across multiple indented lines and stays under 79 chars while
preserving the same logic for is_session_ready, session.hmac_digest and digest.
- Around line 41-42: Remove the standalone import "from flask import
has_request_context" and add has_request_context to the existing "from flask
import ..." import that already brings in other Flask names (the import that
currently imports current_app/g/session or similar), consolidating all Flask
imports into one statement and deleting the duplicate line.
---
Outside diff comments:
In `@web/pgadmin/utils/session.py`:
- Line 105: Add one more blank line immediately before the declaration of class
CachingSessionManager (the class that subclasses SessionManager) so there are
two blank lines separating this top-level class definition from the previous
top-level code; this will satisfy pycodestyle E302 by ensuring two blank lines
before the CachingSessionManager class declaration.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web/pgadmin/utils/session.py (1)
41-42: Consolidate the duplicateflaskimport into line 29.This was raised in a previous review.
has_request_contextshould be added to the existingfrom flask import ...statement on line 29 instead of being a separate import.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/pgadmin/utils/session.py` around lines 41 - 42, Consolidate the duplicate Flask import by removing the separate "from flask import has_request_context" line and adding has_request_context to the existing "from flask import ..." import statement at the top of the file (the current import that brings in other flask symbols), so only one from-flask import remains; ensure no other references are changed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/pgadmin/utils/session.py`:
- Around line 120-129: The is_session_ready method should guard against _session
being None and catch the correct dict-related exceptions from ManagedSession (a
CallbackDict subclass). Change is_session_ready to return False immediately if
_session is None (or check with "if not _session: return False"), then access
the id safely (e.g., use _session.get('_id') or keep the index access but update
the except clause); replace the existing except (AssertionError, RuntimeError)
with an except that catches KeyError (and optionally TypeError for safety) so
missing keys or wrong types no longer crash when ManagedSession or None is
passed.
---
Duplicate comments:
In `@web/pgadmin/utils/session.py`:
- Around line 41-42: Consolidate the duplicate Flask import by removing the
separate "from flask import has_request_context" line and adding
has_request_context to the existing "from flask import ..." import statement at
the top of the file (the current import that brings in other flask symbols), so
only one from-flask import remains; ensure no other references are changed.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
web/setup.py (3)
240-240:⚠️ Potential issue | 🟡 MinorRemove the extraneous
fprefix — no placeholders in the string (Ruff F541).🛠️ Proposed fix
- print(f"Skipping user: missing 'username' or 'email'") + print("Skipping user entry: missing 'username' or 'email'")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/setup.py` at line 240, The print call currently uses an unnecessary f-string: change the call printing "Skipping user: missing 'username' or 'email'" to a plain string (remove the leading f) so it’s not an f-string; locate the print statement in web/setup.py that contains "Skipping user: missing 'username' or 'email'" and replace print(f"...") with print("...").
293-296:⚠️ Potential issue | 🟡 MinorReplace hardcoded
6withconfig.PASSWORD_LENGTH_MIN.The minimum password length is a configurable constant; hard-coding
6here will silently diverge from the configured policy. The error message should reference the same value. This was noted in a prior review cycle but remains unaddressed.🛠️ Proposed fix
- if len(user_data['newPassword']) < 6: + if len(user_data['newPassword']) < config.PASSWORD_LENGTH_MIN: print(f"Skipping user '{user_data['username']}': " - f"password must be at least 6 characters") + f"password must be at least {config.PASSWORD_LENGTH_MIN} characters")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/setup.py` around lines 293 - 296, Replace the hard-coded literal 6 with the configured constant config.PASSWORD_LENGTH_MIN in the password-length check and the printed error message: use config.PASSWORD_LENGTH_MIN when comparing user_data['newPassword'] length and include that same value in the f-string message so the message and policy remain synchronized; ensure config is imported/available in the scope where user_data, user_data['newPassword'], and error_count are used.
208-208:⚠️ Potential issue | 🟡 MinorSpecify explicit encoding when opening the file.
open(file_path)relies on the platform default encoding, which can differ between environments (e.g., Windows vs. Linux). This was noted in a prior review cycle but remains unaddressed.🛠️ Proposed fix
- with open(file_path) as f: + with open(file_path, encoding='utf-8') as f:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/setup.py` at line 208, The file open call using "with open(file_path) as f:" relies on platform default encoding; change this to specify an explicit encoding (e.g., open(file_path, encoding="utf-8")) so file reads are deterministic across environments—update the "with open(file_path) as f:" occurrence accordingly and ensure any other reads in the same module use the same explicit encoding convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/setup.py`:
- Around line 309-311: The current except Exception in the user-processing loop
swallows too many errors and prints the exception using str(e); narrow the catch
to the expected error types (e.g., ValueError, TypeError, and your ORM
integrity/DB exception class used by create_user) instead of a blanket
Exception, and update the log to use the explicit conversion flag (e!s) when
formatting the error message; locate the try/except around the create_user /
user-processing logic in web/setup.py and replace the broad except with a tuple
of specific exceptions and change the print formatting to include {e!s}.
---
Duplicate comments:
In `@web/setup.py`:
- Line 240: The print call currently uses an unnecessary f-string: change the
call printing "Skipping user: missing 'username' or 'email'" to a plain string
(remove the leading f) so it’s not an f-string; locate the print statement in
web/setup.py that contains "Skipping user: missing 'username' or 'email'" and
replace print(f"...") with print("...").
- Around line 293-296: Replace the hard-coded literal 6 with the configured
constant config.PASSWORD_LENGTH_MIN in the password-length check and the printed
error message: use config.PASSWORD_LENGTH_MIN when comparing
user_data['newPassword'] length and include that same value in the f-string
message so the message and policy remain synchronized; ensure config is
imported/available in the scope where user_data, user_data['newPassword'], and
error_count are used.
- Line 208: The file open call using "with open(file_path) as f:" relies on
platform default encoding; change this to specify an explicit encoding (e.g.,
open(file_path, encoding="utf-8")) so file reads are deterministic across
environments—update the "with open(file_path) as f:" occurrence accordingly and
ensure any other reads in the same module use the same explicit encoding
convention.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes